-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Agreement: Add warning when decoding proposal of an unsupported consensus version #3730
Agreement: Add warning when decoding proposal of an unsupported consensus version #3730
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you approach would work correctly for future cases, but not for past cases, since the decoder is still going to fail.
I would suggest a slightly more conservative approach of limiting the change to the demux, and testing against the latest round supported by the ledger.
How do I limit the change to the demux if I need to extract the round number of the provided proposal? And second, when you say testing against the latest round supported by the ledger, do you mean just check if the specified round > max supported round? The issue I'm after is when the local node is already behind, and cannot move forward since it's not supporting the newer protocols. In this case, using |
Codecov Report
@@ Coverage Diff @@
## master #3730 +/- ##
==========================================
- Coverage 49.68% 49.66% -0.03%
==========================================
Files 392 392
Lines 68588 68595 +7
==========================================
- Hits 34076 34065 -11
- Misses 30766 30779 +13
- Partials 3746 3751 +5
Continue to review full report at Codecov.
|
agreement/demux.go
Outdated
cv, err := d.ledger.ConsensusVersion(d.ledger.NextRound()) | ||
if err != nil { | ||
if _, ok := config.Consensus[cv]; !ok { | ||
d.log.Warnf("received proposal with unsupported consensus version: %v", cv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here looks correct. Two small tweaks:
- if you're writing this new log message, then the other log message is no longer needed.
- could you change the log message to something like
d.log.Warnf("received proposal message was ignored. The node binary doesn't support the next network consensus (%v) and would no longer be able to process agreement messages", cv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. I'd rather having these two identical cases merged, but logically this looks correct.
Whats a better way of writing this? I thought about
|
I was thinking about something like this, just to keep the current flow as is : warnMsg := fmt.Printf("disconnecting from peer: error decoding message tagged %v: %v", tag, err)
// check protocol version
cv, err := d.ledger.ConsensusVersion(d.ledger.NextRound())
if err == nil {
if _, ok := config.Consensus[cv]; !ok {
warnMsg = fmt.Printf("received proposal message was ignored. The node binary doesn't support the next network consensus (%v) and would no longer be able to process agreement messages", cv)
}
}
d.log.Warn(warnMsg) |
Summary
When the agreement receives a proposal from a consensus version it cannot yet support, the node disconnects from the sender and logs the fact that it was unable to decode the proposal without providing guidance on the consensus version issue. This PR adds a warning message to the log specifying that the agreement cannot handle the consensus version's proposals